@prudhvigodithi @swoehrl-mw Greetings! I'm an SRE with the Wikimedia Foundation and I work with @brouberol .

We're rolling out a new OpenSearch environment on K8s in the next month or so and I was wondering if y'all had the cycles to review this change? There's more context in our task tracker if y'all are interested.

Thanks for taking a look and feel free to ping here or in OpenSearch Slack if you have any questions or comments.

Adding @patelsmit32123 @synhershko to please take a look and add your thoughts.

We're rolling out a new OpenSearch environment on K8s in the next month or so and I was wondering if y'all had the cycles to review this change? There's more context in our task tracker if y'all are interested.

FWIW we are planning a massive release of a 3.0 version of this operator which will be significantly better and safer to use in production.

What is the use-case for doing what you are doing here? I might be missing something

cc @josedev-union

What is the use-case for doing what you are doing here?

The use case is mostly deployment convenience (only having to deploy a single operator cluster-wide), as well as aligning with common operator behavior within the Kubernetes ecosystem.

For example, having a single operator being able to watch multiple operators is supported by:

• CloudnativePG
• Spark-operator
• Elasticsearch-operator
• flink-operator
• Strimzi
• redis-operator
• zookeeper-operator
• clickhouse-operator
• stackgres
• grafana operator

Note: We're not actively running all of these operators (only a subset), but I sampled actively maintained operator codebases and documentation to showcase that this is a common behavior.

My point (and IMHO the general sentiment over at #374) is that this behavior is expected by operator users and deployers, as it's become quite standard.

It is something that is natively supported by the operator SDK, and does not take anything away from the current operator behavior, as it only adds the ability to have the operator manage one to many clusters, instead of a single one at the moment.

I hope it clears things up.

Note:: I just realized that for this patch to be complete, it's lacking iterating over watched namespaces to setup the appropriate roles and role bindings in each of them. I'm happy to send that work over if the feature request intention is approved.

Got it. Happy to merge this once conflicts are resolved and CI is green.

@josedev-union I'm confused by the useRoleBindings required change.

The chart defines 2 ClusterRoles:

• {{ include "opensearch-operator.fullname" . }}-{{ .Release.Namespace }}-proxy-role
• {{ include "opensearch-operator.fullname" . }}-{{ .Release.Namespace }}-manager-role

If we define cluster.watchNamespace to, say, [ns1, ns2], we probably want to use RoleBindings to bing each of these ClusterRoles, scoped in each of these 2 namespaces, don't we? The alternative would be to use a ClusterRoleBinding which would allow the opensearch operator access to, amongst other things, access to Secret resources in all namespaces.

Cf https://kubernetes.io/docs/reference/access-authn-authz/rbac/#rolebinding-and-clusterrolebinding

A RoleBinding may reference any Role in the same namespace. Alternatively, a RoleBinding can reference a ClusterRole and bind that ClusterRole to the namespace of the RoleBinding. If you want to bind a ClusterRole to all the namespaces in your cluster, you use a ClusterRoleBinding.

(emphasis mine)

The way I see if, the logic should be to use RoleBindings as soon as manager.watchNamespace is non-empty. If it is, then the controller would watch all namespaces, and thus would require a ClusterRoleBinding.

WDYT?

• This is not directly linked to your changes but good to have in the same scope.
  useRoleBindings should be enabled only when watchNamespace is equal to the release namespace. If not, then we need to use clusterRoleBindings.
  Please update the helm doc of useRoleBindings part properly and fix typo in the current one.

yes, we need to use clusterRoleBinding in such cases.
What i requested to change is to update helm docs in values file to clearly explain that.
In the current helm docs, it is incorrect. We can just use RoleBinding only when the watchNamespace is a single one and also it is equal to the release namespace. But now it is written that we can use RoleBinding just when watchNamespace is specified.

So, this is where I'm not sure I agree (or maybe we agree and I'm just misunderstanding).

If you watch specific namespaces, by having a non-empty cluster.watchNamespace, then the chart should use a RoleBinding to bind the operator ClusterRole, in each of the watched namespaces.

The RoleBinding / ClusterRoleBinding decision should be:

flowchart TD
    B{Is it manager.watchNamespace empty?}
    B -- Yes --> C[Use a ClusterRoleBinding to give permissions to the operator on *all* namespaces]
    B -- No ----> D[Use a RoleBinding on the operator ClusterRoleBinding in each of the watched namespaces]

Happy to hear your thoughts

@brouberol nah, it is much more than my original thought. :)
#1101 (review)
I just recommend you to update comments in the helm chart values file only like this.

## If this is set to true, RoleBindings will be used instead of ClusterRoleBindings, inorder to restrict ClusterRoles 
 ## to the namespace where the operator and OpenSearch cluster are in.
 ## You need to set the release namespace as manager.watchNamespace
 useRoleBindings: false 

Ok, I think I pinpointed where our lack of understanding is coming from. Let me know if I get this right.

The opensearch operator is usually deployed in the same namespace than the opensearch cluster. When that is the case, we can use a RoleBinding because the Role will be part of the same namespace as the cluster.
When that is not the case, the current chart design is to use ClusterRole for the operator and a ClusterRoleBinding to grant permission to the operator to all namespaces.

In that light, I understand your comment: having useRoleBindings: true would only work if {{ .Release.Namespace }} == {{ $watchedNamespace }}. If this is how the chart wants things to work, then sure, I'll update the comment.

What I was pushing towards though, is considering that the operator can be run from its own namespace (as it is common in the ecosystem to run operator wither from kube-system or dedicated namespaces, such as opensearch-operator in our case). The operator would define its permissions via a ClusterRole, and the operator permissions would be bound to the watched namespaces only via a ClusterRoleBinding in each of these namespaces (in the following diagram, [ns1, ns2].

classDiagram

    ClusterRole <|-- RoleBindingNS1
    ClusterRole <|-- RoleBindingNS2
    
    class ClusterRole {
        name: opensearch-operator-manager-role
        permissions: [...]
    }
    class RoleBindingNS1 {
        namespace: ns1
        ---
        roleRef.apiGroup: rbac.authorization.k8s.io
        roleRef.kind: ClusterRole
        roleRef.name: opensearch-operator-manager-role
        subjects[0].kind: ServiceAccount
        subjects[0].name: opensearch-operator
        subjects[0].namespace: opensearch-operator
    }
    class RoleBindingNS2 {
        namespace: ns2
        ---
        roleRef.apiGroup: rbac.authorization.k8s.io
        roleRef.kind: ClusterRole
        roleRef.name: opensearch-operator-manager-role
        subjects[0].kind: ServiceAccount
        subjects[0].name: opensearch-operator
        subjects[0].namespace: opensearch-operator    }

The main reason to do this would be to avoid granting the opensearch-operator to read/write/delete/update Secret resources in all namespaces, even those it is not watching, which is as large of a security risk as you can get.

I'm happy to get told "let's punt this to another PR" and I'll just update the comment. However, let's be aware than it its current state, the operator permissions are wide open.

Linking back to #374 original message, we see

For security reasons we are not able to use the clusterrolebinding and have to use namespaced rolebindings instead. This means we can only use the watchNamespace mode of deployment which obviously means that we would have to install multiple operators for multiple clusters in separate namespaces.

This is the security issue they're mentioning.

Ok, I think I pinpointed where our lack of understanding is coming from. Let me know if I get this right.

The opensearch operator is usually deployed in the same namespace than the opensearch cluster. When that is the case, we can use a RoleBinding because the Role will be part of the same namespace as the cluster. When that is not the case, the current chart design is to use ClusterRole for the operator and a ClusterRoleBinding to grant permission to the operator to all namespaces.

In that light, I understand your comment: having useRoleBindings: true would only work if {{ .Release.Namespace }} == {{ $watchedNamespace }}. If this is how the chart wants things to work, then sure, I'll update the comment.

What I was pushing towards though, is considering that the operator can be run from its own namespace (as it is common in the ecosystem to run operator wither from kube-system or dedicated namespaces, such as opensearch-operator in our case). The operator would define its permissions via a ClusterRole, and the operator permissions would be bound to the watched namespaces only via a ClusterRoleBinding in each of these namespaces (in the following diagram, [ns1, ns2].

classDiagram

    ClusterRole <|-- RoleBindingNS1
    ClusterRole <|-- RoleBindingNS2
    
    class ClusterRole {
        name: opensearch-operator-manager-role
        permissions: [...]
    }
    class RoleBindingNS1 {
        namespace: ns1
        ---
        roleRef.apiGroup: rbac.authorization.k8s.io
        roleRef.kind: ClusterRole
        roleRef.name: opensearch-operator-manager-role
        subjects[0].kind: ServiceAccount
        subjects[0].name: opensearch-operator
        subjects[0].namespace: opensearch-operator
    }
    class RoleBindingNS2 {
        namespace: ns2
        ---
        roleRef.apiGroup: rbac.authorization.k8s.io
        roleRef.kind: ClusterRole
        roleRef.name: opensearch-operator-manager-role
        subjects[0].kind: ServiceAccount
        subjects[0].name: opensearch-operator
        subjects[0].namespace: opensearch-operator    }

Loading

The main reason to do this would be to avoid granting the opensearch-operator to read/write/delete/update Secret resources in all namespaces, even those it is not watching, which is as large of a security risk as you can get.

I'm happy to get told "let's punt this to another PR" and I'll just update the comment. However, let's be aware than it its current state, the operator permissions are wide open.

Linking back to #374 original message, we see

For security reasons we are not able to use the clusterrolebinding and have to use namespaced rolebindings instead. This means we can only use the watchNamespace mode of deployment which obviously means that we would have to install multiple operators for multiple clusters in separate namespaces.

This is the security issue they're mentioning.

I prefer to open a new issue for this.
Normally, operators have a boolean flag watchGlobal and they just switch between ClusterRoleBinding and RoleBinding. In this case, the current implementation is ok. But now we specify the list of namespaces, so need to revist this topic to follow PoLP.